Refactor SQL module to use PreparedStatement (#1611) #1612
Conversation
|
Thanks for contributing @ferCancholaCruz Can you please reinstate the license headers and comments that your PR removed? Thanks! |
There was a problem hiding this comment.
To be honest, I do not really get why do you changed the single StringBuilder query to StringBuilder fieldsBuilder, StringBuilder placeholdersBuilder and StringBuilder updatesBuilder with the overcomplicated query building? One single for loop appends different parts to different StringBuilders. The original version was already a PreparedStatement. I can't see any additional value replacing it. Finally it is the same SQL query generated with more resource is used.
Am I missing something?
There was a problem hiding this comment.
The original intention of the issue was to replace direct String concat with parameter replacements and pre-build queries. The SQL strings could be prepared on init and just looked up (might open the was for a more sophisticated solution using a connection pool).
Side note: One would need to sanitize table names, too, because they cannot be set as ? In a prepared statement, so might be good to add a simple check.
| query.append(", ").append((String) o); | ||
| } | ||
| // Build SQL statement with prepared statement | ||
| StringBuilder fieldsBuilder = new StringBuilder(fieldNameForURL()); |
There was a problem hiding this comment.
I agree with @sigee here. We shouldn't create a lot of StringBuilders here and instead use parameter replacement were needed. In addition, I would add a simple (regex) check for the table name to avoid anything unexpected.
|
closed as inactive, feel free to reopen @ferCancholaCruz |
4 participants
This PR refactors parts of the SQL module to use
PreparedStatementinstead of SQL string concatenation, in accordance with issue #1611.✅ Refactored:
SQLSpout.java: replaced string-based query building withPreparedStatementIndexerBolt.java: refactored dynamic insert with metadata into a clean, safePreparedStatement☑️
StatusUpdaterBolt.javaalready usedPreparedStatementcorrectly and did not require changes.The module compiles cleanly with
mvn clean install -DskipTestsand follows recommended SQL practices.Closes #1611